Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Preconditioner and Factorization config #1479

Merged
merged 11 commits into from
May 28, 2024
Merged

Preconditioner and Factorization config #1479

merged 11 commits into from
May 28, 2024

Conversation

yhmtsai
Copy link
Member

@yhmtsai yhmtsai commented Nov 30, 2023 •
edited
Loading

This pr contains the config interface for preconditioner and factorization.

TODO:

  • how to distinguish the ILU between preconditioner and factorization. I currently use Factorization as prefix. (it is only internal)

@yhmtsai yhmtsai self-assigned this Nov 30, 2023
@ginkgo-bot ginkgo-bot added reg:build This is related to the build system. reg:testing This is related to testing. mod:core This is related to the core module. type:preconditioner This is related to the preconditioners type:factorization This is related to the Factorizations reg:helper-scripts This issue/PR is related to the helper scripts mainly concerned with development of Ginkgo. labels Nov 30, 2023
@yhmtsai yhmtsai force-pushed the prec_fact_config branch 2 times, most recently from 0b19c10 to 91840c1 Compare November 30, 2023 23:21
@sonarcloud SonarCloud
Copy link

sonarcloud bot commented Dec 1, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 41 Code Smells

38.7% 38.7% Coverage
0.0% 0.0% Duplication

warning The version of Java (11.0.3) you have used to run this analysis is deprecated and we will stop accepting it soon. Please update to at least Java 17.
Read more here

@yhmtsai yhmtsai force-pushed the solver_config branch 2 times, most recently from 4933ac5 to 2d5fed4 Compare March 28, 2024 12:51
@MarcelKoch MarcelKoch added this to the Ginkgo 1.8.0 milestone Apr 5, 2024
@yhmtsai yhmtsai added the 1:ST:ready-for-review This PR is ready for review label May 13, 2024
@yhmtsai yhmtsai force-pushed the prec_fact_config branch 2 times, most recently from b598f26 to 367d615 Compare May 13, 2024 14:37
MarcelKoch
MarcelKoch requested changes May 14, 2024
View reviewed changes
Copy link
Member

@MarcelKoch MarcelKoch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm. The same comment on array parameters as in #1480 apply. I would also suggest a uniform error message if an unsupported string value is provided.

core/factorization/cholesky.cpp Show resolved Hide resolved
core/config/config_helper.hpp Outdated Show resolved Hide resolved
core/config/config_helper.hpp Outdated
* specialize for const LinOpFactory
*/
template <typename T>
inline deferred_factory_parameter<typename T::Factory> get_specific_factory(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is this different from get_factory followed by a dynamic cast?
If not, this function can be simplified.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It has the specific type directly, so it will go the the type::parse without the type or value_type

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess this should also be renamed?

core/config/config_helper.hpp Outdated Show resolved Hide resolved
core/config/preconditioner_config.cpp Show resolved Hide resolved
core/preconditioner/jacobi.cpp Outdated
// [[x, y], ...] -> array mode
if (auto& obj = config.get("storage_optimization")) {
// only support value mode
// TODO: more than one precision_reduction -> array mode.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same, array mode should not be supported

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

although it can accept shorter list without knowing the matrix size, I remove it now.

core/test/config/preconditioner.cpp Outdated Show resolved Hide resolved
core/test/config/preconditioner.cpp Outdated Show resolved Hide resolved
core/test/config/preconditioner.cpp Outdated Show resolved Hide resolved
core/test/config/preconditioner.cpp Outdated Show resolved Hide resolved
@yhmtsai yhmtsai mentioned this pull request May 14, 2024
Merged
1 task
@yhmtsai yhmtsai force-pushed the prec_fact_config branch 2 times, most recently from 57d3df4 to 0b77935 Compare May 14, 2024 22:47
thoasm
thoasm reviewed May 26, 2024
View reviewed changes
Copy link
Member

@thoasm thoasm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would like a short discussion about the IC and ILU preconditioner. The rest looks good to me!

core/config/config_helper.hpp Outdated Show resolved Hide resolved
core/config/config_helper.hpp Outdated Show resolved Hide resolved
core/preconditioner/ic.cpp Outdated Show resolved Hide resolved
core/preconditioner/ilu.cpp Outdated Show resolved Hide resolved
include/ginkgo/core/factorization/cholesky.hpp Outdated Show resolved Hide resolved
@MarcelKoch
Copy link
Member

@thoasm can you either approve the PR or request changes?

thoasm
thoasm requested changes May 27, 2024
View reviewed changes
core/preconditioner/ic.cpp Outdated Show resolved Hide resolved
Base automatically changed from solver_config to develop May 27, 2024 18:56
MarcelKoch
MarcelKoch approved these changes May 28, 2024
View reviewed changes
include/ginkgo/core/preconditioner/parse_limit.hpp Outdated Show resolved Hide resolved
include/ginkgo/core/preconditioner/parse_limit.hpp Outdated Show resolved Hide resolved
@yhmtsai
Copy link
Member Author

yhmtsai commented May 28, 2024

I wonder whether the instantiation of preconditioner Ic/Ilu in config will leads the duplicated symbol if user also use the same instantiation in their own code.
For function, we can use inline. but I was not sure for the class.

from https://en.cppreference.com/w/cpp/language/inline

A function defined entirely inside a class/struct/union definition, whether it's a member function or a non-member friend function, is implicitly an inline function unless it is attached to a named module(since C++20).

should it be safe?

@upsj
Copy link
Member

upsj commented May 28, 2024 •
edited
Loading

For function, we can use inline. but I was not sure for the class.

template functions automatically emit weak symbols, so we don't need to worry about that

Classes only emit symbols for their member functions and static member variables and things like the vtable, but there is no symbol for the class itself.

@yhmtsai @MarcelKoch
rename, update doc
4e94934 
Co-authored-by: Marcel Koch <[email protected]>
Co-authored-by: Thomas Grützmacher <[email protected]>
@yhmtsai yhmtsai requested a review from thoasm May 28, 2024 12:25
thoasm
thoasm approved these changes May 28, 2024
View reviewed changes
Copy link
Member

@thoasm thoasm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well done extending the implementation without considerable changes to our build system or public headers!
I would like to have one more name change. The rest looks good to me!

include/ginkgo/core/preconditioner/utils.hpp Show resolved Hide resolved
@yhmtsai yhmtsai added 1:ST:ready-to-merge This PR is ready to merge. and removed 1:ST:ready-for-review This PR is ready for review labels May 28, 2024
@yhmtsai yhmtsai merged commit 1c6d79f into develop May 28, 2024
11 of 15 checks passed
@yhmtsai yhmtsai deleted the prec_fact_config branch May 28, 2024 19:13
@sonarcloud SonarCloud
Copy link

sonarcloud bot commented May 29, 2024

Quality Gate Failed Quality Gate failed

Failed conditions
C Reliability Rating on New Code (required ≥ A)

See analysis details on SonarCloud

Catch issues before they fail your Quality Gate with our IDE extension SonarLint

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@thoasm thoasm thoasm approved these changes

@MarcelKoch MarcelKoch MarcelKoch approved these changes

Assignees

@yhmtsai yhmtsai

Labels
1:ST:ready-to-merge This PR is ready to merge. mod:core This is related to the core module. reg:build This is related to the build system. reg:helper-scripts This issue/PR is related to the helper scripts mainly concerned with development of Ginkgo. reg:testing This is related to testing. type:factorization This is related to the Factorizations type:preconditioner This is related to the preconditioners
Projects
None yet
Milestone
Ginkgo 1.8.0
Development

Successfully merging this pull request may close these issues.
5 participants
@yhmtsai @MarcelKoch @upsj @thoasm @ginkgo-bot